Repository navigation
Conversation
🔃 FlexReview StatusCommon Owner:
Review SLO: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --include ancestors flag to the av adopt command, enabling non-interactive adoption of a remote branch and its ancestors. It includes updates to the command-line documentation, enhances the mock GitHub GraphQL server in end-to-end tests to support querying pull requests by number, and adds corresponding test coverage. The feedback suggests registering a shell completion function for the new --include flag to improve the CLI user experience.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| adoptCmd.Flags().StringVar( | ||
| &adoptFlags.Include, "include", "", | ||
| "with --remote, non-interactively adopt the named branch and related branches\n(ancestors)", | ||
| ) |
There was a problem hiding this comment.
To improve the CLI user experience, we can register a shell completion function for the new --include flag. Since the only valid value currently is ancestors (defined in adoptIncludeValues), we can provide this directly to Cobra's shell completion mechanism.
adoptCmd.Flags().StringVar(
&adoptFlags.Include, "include", "",
"with --remote, non-interactively adopt the named branch and related branches\n(ancestors)",
)
_ = adoptCmd.RegisterFlagCompletionFunc(
"include",
func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
return adoptIncludeValues, cobra.ShellCompDirectiveNoFileComp
},
)
av adopt --remote opens an interactive picker, which is awkward for scripts and agents mirroring a coworker's stack. Add --include ancestors to skip the picker and adopt the named branch plus its ancestors up to the trunk in one shot. --dry-run prints the adoption plan. Also extend the e2e harness: the mock GitHub server now answers the pullRequest(number:) query used to walk parent PRs, and mock-pull accepts base=<ref> and body=<file> so a remote stack with av metadata can be modeled in tests.
f364378 to
0efd2fc
Compare
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Include option and validation cmd/av/adopt.go, docs/av-adopt.1.md |
Adds --include, restricts it to ancestors, requires --remote, adds shell completion, and documents the option. |
Non-interactive adoption flow cmd/av/adopt.go, docs/av-adopt.1.md |
Passes the mode to the remote adoption view model. The view model skips the interactive selector and prints the target branches and parent names in dry-run mode. The documentation describes the behavior. |
Remote PR test support e2e_tests/mock_ghgql_server.go, e2e_tests/testscript_test.go |
Adds mock GraphQL lookup by PR number and extends mock-pull to accept base and body options. |
Ancestor adoption end-to-end coverage e2e_tests/testdata/script/adopt_remote_include_ancestors.txtar |
Checks that --include requires --remote, rejects unknown values, and shows the target branch and ancestors in dry-run output. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant AdoptCmd
participant remoteAdoptViewModel
participant Terminal
participant Git
AdoptCmd->>remoteAdoptViewModel: Set nonInteractive from --include
remoteAdoptViewModel->>remoteAdoptViewModel: Skip interactive selector
alt dry-run
remoteAdoptViewModel->>Terminal: Print adoption targets and parent names
else
remoteAdoptViewModel->>Git: Fetch adoption targets
end
Possibly related PRs
- aviator-co/av#749: Changes the remote adoption model interfaces used by the new
--include ancestorspath.
Merge Risk: 
3a160
-
Mergeable with awareness that dry-run can display a different parent from actual adoption when an ancestor PR is closed or merged.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding non-interactive remote adoption of ancestors through the --include ancestors option. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e_tests/mock_ghgql_server.go (1)
141-158: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked type assertion can panic the mock server.
req.Variables["number"].(float64)panics if the variable is missing or of an unexpected type, crashing the mock HTTP handler goroutine with an unclear stack trace instead of a controlled test failure.🛡️ Proposed fix using comma-ok assertion
func (s *mockGitHubServer) handlePRByNumberQuery(req graphqlRequest) graphqlResponse { // JSON numbers decode to float64. - number := int(req.Variables["number"].(float64)) + numberFloat, ok := req.Variables["number"].(float64) + if !ok { + s.t.Fatalf("handlePRByNumberQuery: missing or invalid 'number' variable: %v", req.Variables["number"]) + } + number := int(numberFloat)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_tests/mock_ghgql_server.go` around lines 141 - 158, The handlePRByNumberQuery helper in mockGitHubServer should not use an unchecked type assertion on req.Variables["number"]; it can panic if the variable is missing or not numeric. Update the number extraction to use a comma-ok style check, and if the variable is absent or the wrong type, return a controlled graphqlResponse (or fail the mock request explicitly) instead of crashing the handler. Keep the fix scoped to handlePRByNumberQuery and the req.Variables["number"] lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/av/adopt.go`:
- Around line 336-349: The dry-run preview in adopt.go is using
prInfo.Parent.Name directly, so it can report a stale parent instead of the same
open ancestor/trunk chosen by the real adoption flow. Update the dry-run block
in the adoption command to reuse the same parent-resolution logic as
initAdoption/adopt (the reparent walk) when building the “Would adopt” lines, so
the displayed parent matches what actual adoption would use.
---
Nitpick comments:
In `@e2e_tests/mock_ghgql_server.go`:
- Around line 141-158: The handlePRByNumberQuery helper in mockGitHubServer
should not use an unchecked type assertion on req.Variables["number"]; it can
panic if the variable is missing or not numeric. Update the number extraction to
use a comma-ok style check, and if the variable is absent or the wrong type,
return a controlled graphqlResponse (or fail the mock request explicitly)
instead of crashing the handler. Keep the fix scoped to handlePRByNumberQuery
and the req.Variables["number"] lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6183aa94-6e30-4d9c-a833-0b28366cd89c
📒 Files selected for processing (5)
cmd/av/adopt.godocs/av-adopt.1.mde2e_tests/mock_ghgql_server.goe2e_tests/testdata/script/adopt_remote_include_ancestors.txtare2e_tests/testscript_test.go
| if adoptFlags.DryRun { | ||
| parents := make(map[string]string, len(prs)) | ||
| for _, prInfo := range prs { | ||
| parents[prInfo.Name] = prInfo.Parent.Name | ||
| } | ||
| lines := []string{"Would adopt:"} | ||
| for _, target := range adoptionTargets { | ||
| lines = append(lines, fmt.Sprintf(" %s (parent: %s)", target.Short(), parents[target.Short()])) | ||
| } | ||
| return tea.Batch( | ||
| vm.AddView(uiutils.SimpleMessageView{Message: colors.SuccessStyle.Render(strings.Join(lines, "\n"))}), | ||
| tea.Quit, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'PullRequestState(Merged|Closed)|Parent\.Trunk|prMap\[' cmd/av/adopt.goRepository: aviator-co/av
Length of output: 665
🏁 Script executed:
#!/bin/bash
sed -n '330,450p' cmd/av/adopt.goRepository: aviator-co/av
Length of output: 3934
🏁 Script executed:
#!/bin/bash
sed -n '250,340p' cmd/av/adopt.goRepository: aviator-co/av
Length of output: 2620
🏁 Script executed:
#!/bin/bash
rg -n "type RemotePRInfo|NewGetRemoteStackedPRModel|GetRemoteStackedPRModel|type AdoptingBranch|type BranchState" -S .Repository: aviator-co/av
Length of output: 982
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' internal/actions/get_remote_stacked_pr.go && printf '\n---\n' && sed -n '1,120p' internal/actions/adopt_branches.go && printf '\n---\n' && sed -n '1,120p' internal/meta/branchstate.goRepository: aviator-co/av
Length of output: 9163
Dry-run should use the same reparent walk as adoption. prInfo.Parent.Name can still point to a merged/closed PR, so Would adopt may show a parent that differs from the branch actually adopted after initAdoption walks up to an open ancestor or trunk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/av/adopt.go` around lines 336 - 349, The dry-run preview in adopt.go is
using prInfo.Parent.Name directly, so it can report a stale parent instead of
the same open ancestor/trunk chosen by the real adoption flow. Update the
dry-run block in the adoption command to reuse the same parent-resolution logic
as initAdoption/adopt (the reparent walk) when building the “Would adopt” lines,
so the displayed parent matches what actual adoption would use.
|
/aviator sync |
|
This pull request was synchronized with |
| "adopt branches from remote pull requests, starting from the specified branch", | ||
| ) | ||
| adoptCmd.Flags().StringVar( | ||
| &adoptFlags.Include, "include", "", |
There was a problem hiding this comment.
remote adopt only walks up from the named branch, so the picker only ever shows ancestors, and they're all selected by default. so --include ancestors just means "skip the picker". lets make it a bool like --yes and drop the value list, validation and completion
| if adoptFlags.DryRun { | ||
| parents := make(map[string]string, len(prs)) | ||
| for _, prInfo := range prs { | ||
| parents[prInfo.Name] = prInfo.Parent.Name |
There was a problem hiding this comment.
this can print the wrong parent. initAdoption skips merged/closed parents, so if feat-a is merged, dry-run shows feat-a as the parent but adoption uses main. lets build the branch list in one place and have dry-run print that
| mock-pull feat-a 1 OPEN base=main body=$WORK/feat-a-body.txt | ||
| mock-pull feat-b 2 OPEN base=feat-a body=$WORK/feat-b-body.txt | ||
|
|
||
| exec av adopt --remote feat-b --include ancestors --dry-run |
There was a problem hiding this comment.
this only tests dry-run. can we add a test that actually adopts? push feat-a/feat-b to origin, run without --dry-run, and check the result. a case with feat-a merged would cover the reparent logic too
pls 🙏 my Claude needs non-interactive adoption of entire stacks
What
av adopt --remote <branch>always drops into the interactive picker, which is a wall for scripts and agents trying to mirror a coworker's stack locally. This adds an--include ancestorsflag that skips the picker:It adopts the named branch and its ancestor chain up to the trunk without prompting. Bare
--remotestill opens the picker, so this is non-breaking.--includeis a validated string flag (onlyancestorstoday) so it can grow todescendants/stacklater without adding new flags.Tests
Extends the e2e harness, which had no coverage of the remote-adopt path:
pullRequest(number:)query used to walk parent PRsmock-pullacceptsbase=<ref>andbody=<file>so a remote stack with av stack metadata can be modeledNew testscript covers the
--dry-runplan and--includevalidation.Stacked on #755.
🤖 Generated with Claude Code
Summary by CodeRabbit
--include=ancestors. This option requires--remote; unsupported values are rejected.--dry-runto view the branches and parent relationships that would be adopted without applying the changes. Remote adoption remains interactive when--includeis omitted.av adoptcommand reference with the new option and its behavior.